Retain UI phase items from frame to frame, and consolidate the UI queuing systems into one. - #25290
Retain UI phase items from frame to frame, and consolidate the UI queuing systems into one.#25290pcwalton wants to merge 7 commits into
Conversation
queuing systems into one. Although PR bevyengine#24893 added retention for UI render world instances themselves, in order to avoid re-extracting them from the main world ECS every frame, we still recreate the `TransparentUi` phase items every frame via `add_transient()`, which additionally removes them from the phase at the end of every frame. This is a significant CPU time sink and isn't the preferred pattern in Bevy nowadays. This commit makes the UI-related phase items retained just as 3D meshes are. All `queue_` methods in `bevy_ui_render` have been updated to walk the list of changed and removed meshes and update elements in the `SortedRenderPhase` only as necessary. The calls to `add_transient()` have been removed in favor of the more modern `add_retained()`. Additionally, all the custom queuing systems have been consolidated into a single generic system, `queue_ui_items`. The resources that hold extracted UI items have likewise been consolidated into a generic `UiRenderObjects` resource. The behavior specific to each individual item type (normal UI nodes, box shadows, gradients, etc.) has been factored into a trait named `UiRenderObject`. This has resulted in dramatic simplifications throughout UI rendering. See the documentation for more information. On `many_buttons`, this PR reduces the median frame time from 36.95 ms to 22.78 ms, or 27 FPS to 44 FPS. The `queue_uinodes` system has gone from 6.21 ms/frame to 15.2 μs/frame, a 409× speedup. And, because the Rust standard library's sorting algorithm is good at sorting data that's close to already sorted, the `sort_phase_system` time decreases from 5.17 ms/frame to 1.29 ms/frame, a 4.01× speedup.
pcwalton
added
C-Performance
pcwalton
added
the
S-Needs-Review
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
There was a problem hiding this comment.
Overall, everything looks really good.
There might be some ideas from #25289 that the UiRenderObjects won't map to, I think. For instance with debug outlines, it uses a MainEntityHashMap<ExtractedDebugOutline> map. Because it's just a stack of lines, it only needs to be a single phase item with one representative render entity. That PR isn't as well thought out as this one though, maybe there's some way to make it work with UiRenderObject. And these changes don't force us to use the UiRenderObjects API in every case if we need some flexibility.
Mostly everything is very simple and obviously correct. I found one bug: it seems like renderable objects, apart those extracted into ExtractedUiNodes, aren't drawn unless they are reupdated after spawning.
The gradients example makes it clear:
cargo run --example gradients
The static gradients on the left aren't visible, only the animated nodes on the right are rendered. Pressing the "previous" or "next" buttons updates the gradients, then they become visible and remain visible.
Similarly, with:
cargo run --example box_shadow --features="bevy_feathers"
Initially the shadow is missing. But after changing any of the options in the menu, the shadow appears and then remains visible.
| pipeline_key_builder: E::create_view_pipeline_key_builder(pipeline_key_builder_item), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The lib module is getting a bit big, this could be moved into its own module. Either that or we could have a uinode module and move lib's extract_* and prepare_uinodes there, along with all the associated types.
There was a problem hiding this comment.
Do you want me to do that in this PR? I usually try to keep refactorings in separate PRs to make reviewability easier and so that if this PR gets reverted for whatever reason the refactoring will remain.
There was a problem hiding this comment.
To be clear, I have no problem with moving the code I added into its own module, but moving the code I added plus additional pre-existing code into a module seems like it'd make this PR noisy. I'm happy to do it in a follow-up though.
There was a problem hiding this comment.
Yeah that's sensible, just move the new code for now I think. It'll make it much easier for a second reviewer, once one shows up.
| .ok() | ||
| .and_then(|default_camera_view| { | ||
| let view = extracted_views.get(default_camera_view.0).ok()?; | ||
| let pipeline_key_builder = render_views.get(default_camera_view.0).ok()?; |
There was a problem hiding this comment.
The per camera view components like BoxShadowSamples and UiAntiAlias are stored on the current camera entity, not the UI view entity pointed at by UiCameraView:
| let pipeline_key_builder = render_views.get(default_camera_view.0).ok()?; | |
| let pipeline_key_builder = render_views.get(this_camera_entity).ok()?; |
You can adjust the shadow samples on the box_shadow example to test when this is working.
There was a problem hiding this comment.
Also, there is a second bug I just realised I think. There is no change detection on these query parameters. In the box_shadow example again, the shadow doesn't update on changing the shadow samples. One of the other parameters has to be changed to trigger an update to see the result of the shadow samples change.
There was a problem hiding this comment.
OK, this looks fixed now.
There was a problem hiding this comment.
Yep, checked it with the change detection UI testbed scene from #25299 and everything seems to work now.
|
It is the same bug, changes to the material asset don't trigger a second update. It becomes visible if you change the window size. |
| .get_mut(&main_entity) | ||
| .iter_mut() | ||
| .flat_map(|(_, gradients)| gradients.drain(..)) | ||
| // If there were any previous gradients for this entity, despawn them |
There was a problem hiding this comment.
This is the problem: If an object already exists in the objects list, it is removed and added to changed. But there is no mechanism to add an object to the changed list on the frame it is spawned. Phase items are added from the changed list, so it doesn't get queued for rendering. On a reupdate though, the object is present in the objects list, so then it can be removed, added to the changed list, and then queued correctly.
There was a problem hiding this comment.
OK, I fixed this by updating all places that manually push things into the objects table to instead call a method add that also adds them to the changed list. Note that this caused some awkwardness in that we have to extract the keys (changed entities) from the changed table in some places so that we aren't mutating a table we're iterating over.
Otherwise the `button` example puts the shadow in the wrong place.
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
|
This is very strange, as that part of the UI testbed works locally! |
|
Phew, the problem with the testbed seems to be what I thought it was: the queuing can happen before the material finishes loading, so we manage to place the object in |
|
This should address all review comments (except for the refactoring one). Note that the PR is a lot bigger now, because I had to change all the extraction code to properly add new UI nodes to |
Maybe we can just remove these pipeline keys? I was concerned with the size of the per data vertex when I wrote these shaders, but that seems really naive now. It's not like users commonly draw thousands of gradients or shadows. And with instanced rate, it wouldn't be sent per vertex anyway. |
|
I don't mean to do it here though, removing the pipeline keys here would be out of the scope of this PR for sure. |
| let extracted_uinodes = extracted_uinodes.into_inner(); | ||
| let mut camera_mapper = camera_map.get_mapper(); | ||
|
|
||
| changed_entities.extend(extracted_uinodes.changed.keys().copied()); |
There was a problem hiding this comment.
This extend and drain pattern looks wrong to me. There shouldn't be any need for a set as the keys are already unique, could just be a Vec<MainEntity>. Also does each extraction need its own local copy? It looks like it could just be kept in a resource, that's cleared and extended once per frame in extract_uinode_changes.
There was a problem hiding this comment.
Yeah with a single global list on a full reupdate, it's about 30% improvement on the extract schedule and 8% overall:
https://github.com/ickshonpe/bevy/tree/25290-extracted-changes-vec
There was a problem hiding this comment.
As well if the changed list is ordered by stack index, then the extract systems will extract the changed node data in z order. It would need significant further changes to take advantage of the sorted list though, I think.
| )>, | ||
| >, | ||
| camera_map: Extract<UiCameraMap>, | ||
| mut changed_entities: Local<MainEntityHashSet>, |
There was a problem hiding this comment.
| mut changed_entities: Local<MainEntityHashSet>, | |
| mut changed_entities: Local<Vec<MainEntity>>, |
There was a problem hiding this comment.
I think there are some fundamental problems with the way queuing is structured, both here and on main:
- The UI doesn't support render layers and each UI entity can only have a single camera target. Instead of extracting all the object data into a single huge list, the objects should be stored in per camera target sublists.
- UI objects are layered and flat, atm we update on all sorts of changes that shouldn't affect their ordering. Phase items should be requeued only on changes to their corresponding render object's local stack ordering, visibility, and camera target.
- For uinodes on the same view, root uinodes should only be ordered relative to other roots, and the non-root uinodes should only be ordered relative only to other non-roots belonging to the same root. UI objects should be grouped together by main entity and ordered in fixed local layers.
- It would take some wrangling, but it looks like it should be possible to consolidate everything into a single queue function. Then it could walk top-down: extracted views -> roots of each UI tree on that view -> changed main node entities for that tree -> each class of UI render object in the fixed within node z layer order.
Fine with merging this first though, it's still a substantial improvement on main, then exploring further improvements in follow up PRs
| ) where | ||
| E: UiRenderObject, | ||
| <E::SpecializedRenderPipeline as SpecializedRenderPipeline>::Key: Send + Sync, | ||
| { |
There was a problem hiding this comment.
Not sure if it will make that much difference, but maybe we should have an initial early out check here that returns if the buffers are all empty.
2 participants
Although PR #24893 added retention for UI render world instances themselves, in order to avoid re-extracting them from the main world ECS every frame, we still recreate the
TransparentUiphase items every frame viaadd_transient(), which additionally removes them from the phase at the end of every frame. This is a significant CPU time sink and isn't the preferred pattern in Bevy nowadays.This commit makes the UI-related phase items retained just as 3D meshes are. All
queue_methods inbevy_ui_renderhave been updated to walk the list of changed and removed meshes and update elements in theSortedRenderPhaseonly as necessary. The calls toadd_transient()have been removed in favor of the more modernadd_retained().Additionally, all the custom queuing systems have been consolidated into a single generic system,
queue_ui_items. The resources that hold extracted UI items have likewise been consolidated into a genericUiRenderObjectsresource. The behavior specific to each individual item type (normal UI nodes, box shadows, gradients, etc.) has been factored into a trait namedUiRenderObject. This has resulted in dramatic simplifications throughout UI rendering. See the documentation for more information.On
many_buttons, this PR reduces the median frame time from 36.95 ms to 22.78 ms, or 27 FPS to 44 FPS. Thequeue_uinodessystem has gone from 6.21 ms/frame to 15.2 μs/frame, a 409× speedup. And, because the Rust standard library's sorting algorithm is good at sorting data that's close to already sorted, thesort_phase_systemtime decreases from 5.17 ms/frame to 1.29 ms/frame, a 4.01× speedup.